-
Notifications
You must be signed in to change notification settings - Fork 2.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Update golangci configuration #1785
base: main
Are you sure you want to change the base?
Conversation
Thanks @umarcor, can you provide details on why you had/chose to make these changes? Thanks |
4718c13
to
b8690bb
Compare
@marckhouzam I just applied the "defaults" as currently documented in https://golangci-lint.run/usage/linters/. I enabled the ones listed in https://golangci-lint.run/usage/linters/#enabled-by-default, which were not enabled here. By the way, I cleaned up the ones marked as "no longer maintained" or "replaced by". |
@marckhouzam also, the makefile was modified because |
b8690bb
to
10ed1b6
Compare
- varcheck | ||
# Disabled by Default | ||
#- asasalint | ||
#- asciicheck |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't realize our .golangci.yml
had abunch of rules commented out. That seems weird. And makes the linter file harder to read: I don't think anyone really needs to know what rules we explicitly aren't using, that's implied by them not being in the file to begin with.
Can we just remove them?
|
||
fmt: | ||
$(info ******************** checking formatting ********************) | ||
@test -z $(shell gofmt -l $(SRC)) || (gofmt -d $(SRC); exit 1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So we no longer need gofmt
because our linter handles it? We should change this to be a call out to the golangci-lint
commandline utility then.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See the 'lint' target in the same Makefile.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah gotcha - but is golangci-lint
going to format poorly formatted code?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No. It will check if the format is correct. That's what the current fmt
Makefile target does:
Lines 19 to 20 in a281c8b
$(info ******************** checking formatting ********************) | |
@test -z $(shell gofmt -l $(SRC)) || (gofmt -d $(SRC); exit 1) |
This PR does not introduce any change in that regard.
10ed1b6
to
2eb91c8
Compare
|
||
default: all | ||
|
||
all: fmt test |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removing fmt
here means that when people run make
locally, they will no longer get errors for formatting errors. Considering the linter takes less tan 2 seconds to run, I suggest running it for make all
.
If someone does not want to run it, they can do make test
directly.
What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fmt
is removed here because the Makefile target fmt
is removed in this PR. We can add lint
in this target, but that will require contributors to have golangci-lint installed before running make
. I think it is better not to require having golangci-lint in order to make
cobra. At the same time, it does not make sense to have an specific make target for one single linter but not for others. Overall, the users/developers interested in contributing code will always want to make lint test clean
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When I started contributing I would do make
and think everything was ok. It was only later that I realized this did not include the linters, so now I run make lint
manually.
But I agree that making golangc-lint
a requirement may be annoying. But it will also be annoying to only notice the bad formatting once CI is run...
I'm not sure what is best...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMHO modifying the sources and building cobra are not directly related to contributing code back to the upstream. That is, users might want to use and modify this open source tool regardless of the linting rules enforced in this repository. From this point of view, the requirement/need to use golangci-lint belongs to contributing guidelines: https://github.com/spf13/cobra/blob/main/CONTRIBUTING.md. Everyone should read the contributing guidelines before submitting a PR to a project.
2eb91c8
to
27269c7
Compare
27269c7
to
842b246
Compare
842b246
to
fc0698b
Compare
fc0698b
to
161ba4a
Compare
27f6105
to
be6cd55
Compare
22f06f4
to
567f40f
Compare
567f40f
to
2611353
Compare
2611353
to
cee7f37
Compare
No description provided.